4.1 Code Review

正文

Collaborative Code Development Models

Fork and Pull Model

核心特点是：每个贡献者先拥有自己的仓库副本。

基本流程：

原始仓库
→ Fork 到个人账号
→ 在个人仓库修改代码
→ Push 到个人仓库
→ 向原始仓库提交 Pull Request
→ 维护者审查并合并

Shared Repository Model

核心特点是：所有团队成员直接在同一个仓库中工作。

基本流程：

共享仓库
→ 创建 feature branch
→ 修改并 push
→ 从 feature branch 创建 PR
→ 团队审查
→ 合并到 main 或 develop

Several code review techniques

• Over-the-shoulder code review - one developer talks the other developer through the code changes while sitting at the same machine.
• Pair programming - two developers work on the code at the same time with one of them actively coding and the other providing real-time feedback.
• Formal code inspection - up to 6 partipants go through a formalised process to inspect the code specification or design for defects.
• Tool assisted code review - developers use tools such as GitHub to review the code independently and give feedback.

main branch only contains “production-ready” work, and that the development branch contains code that has already been extensively tested.

原文有较多GitHub上面的GUI操作(关于如何创建PR，如何处理PR)。 直接看原始教程, 从"### Raising a Pull Request"开始

Things to Look for in a Code Review

Start by understanding what the code should do, by reading the specification/user requirements, the pull request description or talking to the developer if need be. In this case, understand what SR1.1.1 means.

Once you are happy, start reading the code (skip the test code for now - we will come back to it later). you are going to be assessing the code in the following key areas.

Is the proposed code readable?

• Think about the names of the variables and functions - do they follow naming conventions?
• Do you understand what conditions in each if statements are for?
• Does a function name match the behavior of the function?

Is the proposed code a minimal change?

• Does the code reimplement anything that already exists, either elsewhere in the codebase or in a library you know about?
• Does the code implement something that is not the requirement or in the issue/ticket?

Is the structure of the code clear?

• Do functions do just one thing?
• Is the code using the right level of modularity?
• Is the code consistent with the structure of the rest of the code?

Is there an appropriate and up-to-date documentation for the proposed code?

• If functionality has changed, has corresponding documentation been updated?
• If new functions have been added, do they have the associated documentation?
• Does the documentation make sense?
• Are there clear and useful comments that explain complex designs and focus on the “why/because” rather than the “what/how”?

Things Not to Look for in a Code Review

The overriding priority for reviewing code should be making sure progress is being made - do not let perfect be the enemy of the good here. According to “Best Kept Secrets of Peer Code Review” (Cohen, 2006) the first hour of reviewing code is the most effective, with diminishing returns after that.

To that end, here are a few things you should not be trying to spot when reviewing:

• Linting issues, or anything else that an automated tool can spot - get the Continuous Integration (CI) to do it.
• Bugs - instead make sure there are tests for all cases.
• Issues that pre-date the change - raise separate PRs fixing these issues separately to avoid heading down a rabbit hole.
• Architecture re-writes - try to have design discussions upfront, or else have a meeting to decide whether the code needs to be rewritten.